-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(v2): support custom description for blog-only mode #2359
Conversation
Deploy preview for docusaurus-2 ready! Built with commit 77bacef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good API design. We shouldn't use customFields for a first-class feature. I don't have a good way around this right now but using the siteConfig's description would be better. Ideally we allow setting it on the blog plugin but there's no easy way to pass data from the blog plugin to the front end right now.
OK. I will rename it to Is it the ideal way to let the user write a description for each post at the header option in the markdown file, and set the whole site's description in the |
By examining the source code, I found that the components are dynamically loaded by their filenames. The Here I made some changes (I would consider they are a little bit big.., but you can help me to review it 🤔 ..), here is the description of the major changes I have made:
function ComponentCreator(path, routeProps) {
//...
return <Component {...loadedModules} {...props} {...routeProps} />;
function generateRouteCode(routeConfig: RouteConfig): string {
const {
path: routePath,
component,
modules = {},
routes,
exact,
props,
} = routeConfig;
//...
return `
{
path: '${routePath}',
component: ComponentCreator('${routePath}'${
props ? `, ${JSON.stringify(props)}` : ''
}),
${exactStr}
${routesStr}
}`;
}
addRoute({
path: permalink,
component: blogListComponent,
exact: true,
modules: {...},
props: {
blogSiteDescription,
},
}); I would update the code, so you can have a review of it. |
@zxuqian this feature makes sense to me, can you update your pr? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
willing to merge but need:
- rebase
- remove the "routeProps" system (un-needed)
- rename blogSiteDescription to blogDescription
- document
@@ -149,6 +149,7 @@ export interface RouteConfig { | |||
routes?: RouteConfig[]; | |||
exact?: boolean; | |||
priority?: number; | |||
props?: object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
@@ -43,6 +43,7 @@ const DEFAULT_OPTIONS: PluginOptions = { | |||
remarkPlugins: [], | |||
rehypePlugins: [], | |||
truncateMarker: /<!--\s*(truncate)\s*-->/, // Regex. | |||
blogSiteDescription: 'Blog', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blogDescription seems a better name (+ we need documentation)
@@ -285,6 +287,9 @@ export default function pluginContentBlog( | |||
}), | |||
metadata: aliasedSource(pageMetadataPath), | |||
}, | |||
props: { | |||
blogSiteDescription, | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need that, you'll already receive metadata.blogDescription as props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @slorber , I found that to use blogDescription
in metadata
, I still need to get it from options
object and pass it to the metadata
object here(about line 125):
metadata: {
permalink: blogPaginationPermalink(page),
page: page + 1,
postsPerPage,
totalPages: numberOfPages,
totalCount,
previousPage: page !== 0 ? blogPaginationPermalink(page - 1) : null,
nextPage:
page < numberOfPages - 1
? blogPaginationPermalink(page + 1)
: null,
blogDescription: options.blogDescription,
}
Could you tell me if this is the case? Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the blogDescription is a field on the plugin options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! Thanks.
@@ -13,15 +13,14 @@ import BlogPostItem from '@theme/BlogPostItem'; | |||
import BlogListPaginator from '@theme/BlogListPaginator'; | |||
|
|||
function BlogListPage(props) { | |||
const {metadata, items} = props; | |||
const {metadata, items, blogSiteDescription} = props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use metadata.blogDescription
component: ComponentCreator('${routePath}'), | ||
component: ComponentCreator('${routePath}'${ | ||
props ? `, ${JSON.stringify(props)}` : '' | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
@@ -77,7 +77,7 @@ function ComponentCreator(path) { | |||
|
|||
const Component = loadedModules.component; | |||
delete loadedModules.component; | |||
return <Component {...loadedModules} {...props} />; | |||
return <Component {...loadedModules} {...props} {...routeProps} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
website/docusaurus.config.js
Outdated
@@ -51,6 +51,7 @@ module.exports = { | |||
type: 'all', | |||
copyright: `Copyright © ${new Date().getFullYear()} Facebook, Inc.`, | |||
}, | |||
blogSiteDescription: 'Test description....', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove to be able to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I leave blogDescription inside blog config, or should I move it to the root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's in the blog options, because a Docusaurus might support hosting multiple blogs in the future, each one having its own description
OK! I will update it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good, just a few things to change
@@ -20,6 +20,7 @@ export const DEFAULT_OPTIONS = { | |||
blogTagsListComponent: '@theme/BlogTagsListPage', | |||
blogPostComponent: '@theme/BlogPostPage', | |||
blogListComponent: '@theme/BlogListPage', | |||
blogDescription: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blogDescription: '', | |
blogDescription: 'Blog', |
Let's keep it retrocompatible, it displays Blog by default currently
<Layout title={title} description="Blog"> | ||
<div className="container margin-vert--lg"> | ||
<Layout title={title} description={blogDescription}> | ||
<div className="container margin-vert--xl"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div className="container margin-vert--xl"> | |
<div className="container margin-vert--lg"> |
It's not the purpose of this PR to change the design. I'm open to discussing this but in another PR.
I think it looks weird on mobile to have as much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! This might be accidentally merged from a conflict, I will correct it.
website/docs/blog.md
Outdated
'@docusaurus/preset-classic', | ||
{ | ||
blog: { | ||
blogDescription: 'A docusuarus powered blog!', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blogDescription: 'A docusuarus powered blog!', | |
blogDescription: 'A docusaurus powered blog!', |
website/docs/using-plugins.md
Outdated
/** | ||
* Blog page meta description for better SEO | ||
*/ | ||
blogDescription: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blogDescription: '', | |
blogDescription: 'Blog', |
Looks good, ready to merge, thanks |
No problem, and thank you for the reviews! |
Motivation
In blog-only mode, the meta description is set to "Blog", which leads to poor SEO. The custom homepage under
src/pages/index.js
uses the value of acustomFields.description
field defined indocusaurus.config.js
to display the description. In this PR, I use the samecustomFields.description
config value for theBlogPostList
component in the docusaurus-theme-classic theme.Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
docusaurus.config.js
, look at thecustomFields.description
value:routeBasePath
to'/'
inpresets
customFields
:Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)